Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create tempfile using the basename without extension: #201

Merged
merged 5 commits into from
Mar 27, 2018

Conversation

Edouard-chin
Copy link
Contributor

@Edouard-chin Edouard-chin commented Aug 30, 2017

👋 Hello guys, thanks for creating and maintaining this gem

puts Rack::Test::UploadedFile.new('foo.txt').path # => '/var/xxx/foo.txt2017-30-08-dsad.txt'
  • This PR modifies this behaviour to not include the file extension as part of the file basename when creating the Tempfile

- 5f78451 introduced a change to keep the filename extension when creating the tempfile
- `Tempfile#path` is now returning the filename with the extension followed by random chars and the extension again
```ruby
puts Rack::Test::UploadedFile.new('foo.txt').path # => '/var/xxx/foo.txt2017-30-08-dsad.txt'
```
- This PR modifies this behaviour to not include the file extension as part of the file basename when creating the Tempfile
@Edouard-chin Edouard-chin force-pushed the uploadedfile-double-extension-fix branch from a187321 to 776036e Compare August 30, 2017 21:29
@perlun
Copy link
Contributor

perlun commented Sep 6, 2017

Thanks for the suggestion! To make the change clearer, could you re-formulate the spec to say "file name matches foo" instead of "file name does not match bar"? Or add a comment explaining the expected format.

@Edouard-chin
Copy link
Contributor Author

Edouard-chin commented Sep 6, 2017 via email

@perlun
Copy link
Contributor

perlun commented Sep 20, 2017

Thanks for the feedback ! Will take care on it when I return from vacations (no laptop right now :( )

Take care, no worries for my sake! Enjoy the vacation! 😎

@perlun
Copy link
Contributor

perlun commented Nov 20, 2017

@Edouard-chin Ping - any updates on this?

@perlun perlun added the pending label Nov 20, 2017
@Edouard-chin
Copy link
Contributor Author

Edouard-chin commented Nov 30, 2017

Will take care on it when I return from vacations

Apologize for the time it took me to get back on this, looks like I took too much vacations 😂
I have made the suggested change. The Time.now.year part might be confusing but that's the only way I have found in order to ensure "foo" is not followed by a ".txt" extension. If you prefer I can use a negative lookahead regex

The year is always present in a tmpfile name (ruby Tempfile uses Dir::Tmpname.create)

@olleolleolle
Copy link
Contributor

@perlun I think this is ready for re-review, now.

Copy link
Contributor

@perlun perlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort! I will include this now in the all-great, upcoming release.


regex = /foo#{Time.now.year}.*\.txt\Z/
expect(uploaded_file.path).to match(regex)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is what the test output looks like with this change, but without the actual bug fix:

  1) Rack::Test::UploadedFile creates Tempfiles with a path that includes a single extension
     Failure/Error: expect(uploaded_file.path).to match(regex)

       expected "/var/folders/r8/mlwxnwln1vl5_t_nn90cpr5w0000gn/T/foo.txt20180327-86535-10jssch.txt" to match /foo2018.*\.txt\Z/
       Diff:
       @@ -1,2 +1,2 @@
       -/foo2018.*\.txt\Z/
       +"/var/folders/r8/mlwxnwln1vl5_t_nn90cpr5w0000gn/T/foo.txt20180327-86535-10jssch.txt"

...i.e. foo.txt20180327-86535-10jssch.txt contains the .txt extension twice.

@perlun perlun merged commit 2f37478 into rack:master Mar 27, 2018
alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021
* Create tempfile using the basename without extension:

- 5f78451 introduced a change to keep the filename extension when creating the tempfile
- `Tempfile#path` is now returning the filename with the extension followed by random chars and the extension again
```ruby
puts Rack::Test::UploadedFile.new('foo.txt').path # => '/var/xxx/foo.txt2017-30-08-dsad.txt'
```
- This PR modifies this behaviour to not include the file extension as part of the file basename when creating the Tempfile

* PR Review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants